feat(tools/jira): add write subcommands (comment, transition, label, assign, field, attach)#345
Conversation
a4fae3d to
78b3eec
Compare
andreahlert
left a comment
There was a problem hiding this comment.
Scope is tight and the mock harness is a solid first layer — really like where this is going. Requesting changes on three things that imho will bite the first time a skill calls this against issues.apache.org/jira:
cmd_fieldonly sends string values — most non-trivial JIRA fields (priority, version, user, single-select) need objects/arrays and will 400. See inline.- README auth description is Cloud-flavored but
cmd_assignuses DC{name}. Needs to either say "DC only" or actually handle Cloud. - ASF JIRA PATs use
Authorization: Bearer, not Basic. Today PAT users have to fake Basic viabase64(user:pat).
The rest below are nits / follow-ups — flagging them while I'm here, not blocking on them.
Cross-cutting:
- No retry/backoff anywhere. ASF JIRA does rate-limit; a small
429/5xx → 3 retries w/ backoffloop would save pain in long skill flows. - Pre-existing, but since the README is being touched here:
@Grab('org.apache.groovy:groovy-json:4.0.21')requires Groovy 4 (3.x usesorg.codehaus.groovy), yetREADME.md:66still says "Groovy 3.x or newer". Easy fix in passing.
|
@andreahlert Thanks for the thorough review — addressed all three blocking items and most nits in 2dcfadc. Retry/backoff: Agreed this will matter in long skill flows. It applies equally to the read path, so imho better as a cross-cutting follow-up than scoped to this PR. Groovy version: Fixed — README now says "Groovy 4.x" and explains the |
andreahlert
left a comment
There was a problem hiding this comment.
Thanks for addressing this. LGTM.
potiuk
left a comment
There was a problem hiding this comment.
Thanks for the iteration — implementation is clean and the prior-feedback
round (Bearer scheme, atomic update API, UTF-8 file reads) shows good
attention to detail. Strict key regex (^[A-Z][A-Z0-9_]*-\d+$) closes the
URL-injection path, and the 28 pytest tests cover the right happy + error +
auth surfaces.
Blocking — wire the test suite into CI
The new tools/jira/ is a proper uv project with tests, but neither the
workflow matrix nor the prek hooks pick it up — so the 28 tests land as
dead weight in CI and a future regression would never block a PR. Two small
additions, both mechanical, mirror the pattern the other tools use:
1. Add jira to the pytest matrix in .github/workflows/tests.yml:
project:
- name: oauth-draft
path: tools/gmail/oauth-draft
- name: generate-cve-json
path: tools/vulnogram/generate-cve-json
- name: skill-and-tool-validator
path: tools/skill-and-tool-validator
- name: privacy-llm-checker
path: tools/privacy-llm/checker
- name: privacy-llm-redactor
path: tools/privacy-llm/redactor
- name: vulnogram-oauth-api
path: tools/vulnogram/oauth-api
- name: sandbox-lint
path: tools/sandbox-lint
- name: agent-isolation
path: tools/agent-isolation
- name: jira
path: tools/jira
2. Add prek hooks for jira in .pre-commit-config.yaml, mirroring the
sibling pattern (the skill-and-tool-validator block is the closest analogue
— four hooks: ruff-check, ruff-format, mypy, pytest). Each hook's
files: regex should match ^tools/jira/(src|tests|pyproject\.toml|bridge\.groovy).
Once both land, pytest (jira) shows up as a matrix entry under the
tests-ok umbrella and prek runs the lint pass locally and in CI.
Happy to apply this as a maintainer-side fixup if you'd rather not chase it
— just say the word. Otherwise, push the two edits when you have a moment
and I'll re-review.
This review was drafted by an AI-assisted tool and confirmed by an Apache Steward
maintainer. After you've addressed the points above and pushed an update, an
Apache Steward maintainer will take the next look at your PR.More on how Apache Steward handles maintainer review:
CONTRIBUTING.md.
All four checks pass locally (ruff check, ruff format, mypy, pytest collect — 28 tests). |
…assign, field, attach) Extends the read-only JIRA bridge with six write subcommands that mirror the existing read API shape. Write operations require JIRA_API_TOKEN and follow the same write-path discipline as the GitHub bridge: every mutation is gated on explicit user confirmation in the calling skill. Adds a pytest test harness with a mock HTTP server (24 tests, auto-skipped when groovy is not on PATH). Closes apache#301 Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
- Add JIRA_AUTH_SCHEME env var (Basic/Bearer) for ASF JIRA DC PATs - Add --value-json flag to field subcommand for structured values - Switch label command to atomic JIRA update API (no read-modify-write race) - Use explicit UTF-8 encoding for comment body file reads - Clarify README: DC-only scope, Groovy 4.x requirement, auth docs - Add DC-only comment on assign payload - Add auth header value and Bearer scheme assertions in tests (28 total) Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
Add jira to the pytest matrix in .github/workflows/tests.yml and add prek hooks (ruff check, ruff format, mypy, pytest) in .pre-commit-config.yaml, mirroring the pattern used by the other tool packages. Adds ruff and mypy to dev dependencies. Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
Re-resolve `tools/jira/uv.lock` so the locked `ruff` version matches what CI's `prek` job picks (the framework pins to the most recent release that is at least 7 days old; the committed lock had a newer version that hadn't yet aged in). Generated-by: Claude Code (Opus 4.7)
potiuk
left a comment
There was a problem hiding this comment.
LGTM — the CI wire-in addresses the prior REQUEST_CHANGES exactly:
.github/workflows/tests.yml—jira / tools/jiramatrix entry slots in
alongside the other tool packages; thetests-okumbrella now gates the
jira suite alongside the rest..pre-commit-config.yaml— four hooks under arepo: localblock
(ruff check,ruff format --check,mypy,pytest), each with a
files:regex matching^tools/jira/(src|tests|pyproject\.toml|bridge\.groovy)
exactly as proposed.pyproject.toml—ruffandmypyadded to thedevdep group; uv.lock
reflects.
Verified locally on the rebased branch: ruff check clean, ruff format --check clean, mypy reports no issues, pytest skips the Groovy-dependent
cases (groovy not installed locally) but the test suite's skip semantics
are correct and CI ships groovy. The 28 test assertions still match the
prior round.
The substantive review from earlier still stands: clean Groovy
implementation of six write subcommands; strict key regex (^[A-Z][A-Z0-9_]*-\d+$)
closes URL injection; atomic JIRA update API for labels; DC-only scope
called out; Bearer auth supported.
Thanks for the quick turnaround on the CI wiring.
This review was drafted by an AI-assisted tool and confirmed by an Apache Steward
maintainer. The maintainer approving this PR has read the findings and signed off.
If something feels off, please reply on the PR and a maintainer will follow up.More on how Apache Steward handles maintainer review:
CONTRIBUTING.md.
…erns from session manual cleanups (#402) Per direct observations from the airflow-s 2026-05-29/30 bulk sync — two recurring title-noise patterns were cleaned manually that the existing cascade did not catch: 1. Trailing prior-CVE-relationship parentheticals — the cross-CVE relationship is structurally captured by the Gate #3 cross-CVE clause in the public summary; embedding the relationship in the title is noise to downstream advisory consumers. Catches every shape observed in this session: - `(CVE-YYYY-NNNNN)` - `(possible CVE-YYYY-NNNNN variant)` — from #345 - `(incomplete fix for CVE-YYYY-NNNNN)` — from #351 - `(fix-bypass of CVE-YYYY-NNNNN)` — from #352 - and any other `(... CVE-YYYY-NNNNN ...)` shape 2. Trailing reporter-name attribution parentheticals — reporter attribution lives in the credits field, never in the public title. Pattern matches `(<name> follow-up)` where `<name>` matches name-like tokens (word chars, dots, hyphens, single inline spaces) to avoid over-stripping substantive technical content. Catches: - `(Evan Ricafort follow-up)` — from #346 Substantive technical parentheticals stay intact — e.g. the operator- name list `(GCSToSFTPOperator + GCSTimeSpanFileTransformOperator)` on the GCS path-traversal tracker is NOT stripped (it lacks a CVE ID and doesn't end in `follow-up`). The matching Step 1d signal row in security-issue-sync now enumerates the two new patterns so the proposal-time detector and the pre-push Gate #4 stay in lock-step with the cascade. Validated against 9 cases: 4 session-derived fixes (all pass), 3 synthetic CVE-relationship variants (all pass), 1 substantive technical parenthetical (preserved correctly), 1 "<word> follow-up" edge case (stripped as designed — narrow scope acceptable since "follow-up" titles in airflow-s are exclusively reporter-attribution). Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
comment,transition,label,assign,field, andattachJIRA_API_TOKENand follow the same write-path discipline as the GitHub bridge (mutations gated on explicit user confirmation in the calling skill)Test plan
uv run pytest— 24 tests, skipped when groovy not on PATH)tools/dev/check-placeholders.sh— OK)Closes #301